Use authorization code flow with PKCE when a client secret is specified by N-lson · Pull Request #67 · OctopusDeploy/OpenIDConnectAuthenticationProviders

N-lson · 2022-03-17T23:03:49Z

Manually tested with:

Okta - on initial sign-in, didn't redirect me back to Octopus. had to manually re-open, and on second sign-in was signed into Octopus. I think this is because I then signed into Okta using Google so it can't handle redirecting on a double SSO. Both implicit flow and auth code flow work with this branch, meaning any instances currently using OAuth shouldn't be affected until they add a client secret.
Google - no issues
Azure AD - no issues. the docs say to use a https endpoint and a LetsEncrypt SSL cert, but I tested using http and didn't have a problem signing in . might just be required to set up the logout URL correctly since that requires a https endpoint.

Also comes with some docs changes OctopusDeploy/docs#1496

Testing

Terraform files for creating an app registration in Okta are available on the nelson/terraform branch. You can follow the instructions in our docs for creating a dev Okta instance.

The variables are:

org_name -> The name of the instance eg. dev-3219475 (don't need to add -admin)
api_token -> The API token generated in Security -> API in the Okta portal
redirect_uri -> Would be http://localhost:8065/api/users/authenticatedToken/Okta on a default local server instance

zentron

Couple little requests around code cleanup and a rename of the SessionId to RequestId but otherwise its looking pretty good.

zentron · 2022-04-08T06:31:32Z

+            var stateFromRequest = JsonConvert.DeserializeObject<LoginStateWithSessionId>(state);
+            var response = await RequestAuthToken(code, redirectUri, stateFromRequest!.SessionId);
+
+            // Step 1: Try and get all of the details from the request making sure there are no errors passed back from the external identity provider


Im guessing this is a bit of C+P from what was in the original UserAuthenticatedAction method. is there anything here that we have in common that can be extracted out?

zentron · 2022-04-08T06:34:18Z

+            return loginFailed.Response(message);
+        }
+
+        IResultFromExtension<IUser> GetOrCreateUser(UserResource userResource, string[] groups, CancellationToken cancellationToken)


Again, is there much in here which is really different from the existing class?

N-lson · 2022-04-14T02:22:04Z

+
+namespace Octopus.Server.Extensibility.Authentication.OpenIDConnect.Common.Web
+{
+    public class AuthServerResponseHandler<TAuthTokenHandler> where TAuthTokenHandler : IAuthTokenHandler


Technically this either handles a request (in the case of the implicit flow) or a response (in the case of auth code flow) from the auth server but I went with naming it ResponseHandler.

N-lson · 2022-04-14T02:23:04Z

+
+        public async Task<ClaimsPrincipalContainer> GetDetailsFromRequestEnsuringNoErrors(IDictionary<string, string?> requestForm)
+        {
+            var principalContainer = await authTokenHandler.GetPrincipalAsync(requestForm, out _);


No longer use the out var for the state - could also remove it from the method itself?

N-lson · 2022-04-14T02:24:25Z

+            var octoResponse = UserAuthenticatedValidator.Redirect.Response(state.RedirectAfterLoginTo)
+                .WithHeader("Expires", new[] { DateTime.UtcNow.AddYears(1).ToString("R", DateTimeFormatInfo.InvariantInfo) })
+                .WithCookie(new OctoCookie(UserAuthConstants.OctopusStateCookieName, Guid.NewGuid().ToString()) { HttpOnly = true, Secure = false, Expires = DateTimeOffset.MinValue })
+                .WithCookie(new OctoCookie(UserAuthConstants.OctopusNonceCookieName, Guid.NewGuid().ToString()) {HttpOnly = true, Secure = false, Expires = DateTimeOffset.MinValue});


I'm guessing setting these cookies just resets them to some value so they can't be re-used?

N-lson · 2022-04-14T02:25:43Z


        public async Task<IOctoResponseProvider> ExecuteAsync(IOctoRequest request)
        {
-            // Step 1: Try and get all of the details from the request making sure there are no errors passed back from the external identity provider


I removed all the comments. I think some of the method names are very clear, others maybe it would be useful to keep the comment?

N-lson · 2022-04-14T02:32:47Z

+        {
+            await mediator.Do(
+                new PutBlobCommand(ConfigurationStore.ConfigurationSettingsName, requestId, JsonSerializer.SerializeToUtf8Bytes(blob)),
+                new CancellationToken());


Is new CancellationToken ok here?

I cant see many places where we might already have a token so could you perhaps just wrap the code as high as it makes sense with a

using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(1));

and pass it around where necessary

zentron

Minor comment around the cancellation token but once that is fixed and this all still works then feel free to merge :)
Thanks for the additional time spent 👍

zentron · 2022-04-20T05:39:52Z

+        {
+            await mediator.Do(
+                new PutBlobCommand(ConfigurationStore.ConfigurationSettingsName, requestId, JsonSerializer.SerializeToUtf8Bytes(blob)),
+                new CancellationToken());


I cant see many places where we might already have a token so could you perhaps just wrap the code as high as it makes sense with a

using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(1));

and pass it around where necessary

N-lson added 20 commits March 17, 2022 09:19

Add very basic pkce endpoints and related actions on google auth

2b87d22

First version of code verifier and code challenge

4719713

More pkce stuff

f7440f2

Add client secret

94121e9

use HasClientSecret in AuthTokenHandler.cs

54b03c4

Move Pkce stuff into existing AuthenticationAction

13934ca

Add client_secret to authenticated action

437406e

Use HasClientSecret for everything

c2e3310

A lot of renaming and remove client secret stuff from OctopusID

57a4e3d

code challenge/verifier improvements

b54f2d0

Remove nonce comparison from Pkce action

8806827

Check if state exists

6ba54d3

Validate on id token

028eb91

Clean up

919df03

Updates for Okta

5df8a37

AAD and OctopusID classes

8e15436

remove old store

32740b1

Add actions

a415e81

Use blob storage to store code verifier

096f66c

Update code verifier generation

e79f046

N-lson marked this pull request as ready for review March 30, 2022 00:05

N-lson self-assigned this Mar 30, 2022

N-lson requested a review from a team March 30, 2022 00:27

N-lson commented Mar 30, 2022

View reviewed changes

Comment thread source/Server.OpenIDConnect.Common/Infrastructure/Pkce.cs

zentron suggested changes Apr 8, 2022

View reviewed changes

N-lson added 3 commits April 11, 2022 14:47

Clean up

28ffa70

Update MessageContracts version

e593661

Remove imports

4762155

N-lson requested a review from zentron April 11, 2022 05:04

refactor wip

232a483

Refactor into handler

c2eea8e

N-lson commented Apr 14, 2022

View reviewed changes

Make pkce stuff in authentication action a bit clearer

e9548c1

N-lson commented Apr 14, 2022

View reviewed changes

zentron approved these changes Apr 20, 2022

View reviewed changes

Move cancellation token up

c2f643f

N-lson merged commit f436506 into main Apr 20, 2022

N-lson deleted the nelson/pkce branch April 20, 2022 22:55

N-lson mentioned this pull request Apr 20, 2022

Empty commit to bump minor version #69

Merged

N-lson mentioned this pull request May 6, 2022

Authorization Code Flow (PoC) #52

Closed

Uh oh!

Conversation

N-lson commented Mar 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Uh oh!

Uh oh!

zentron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zentron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

N-lson commented Mar 17, 2022 •

edited

Loading